otdf-sdk-mgr/tests/test_cli_scenario.py (1)

107-126: ⚡ Quick win

Add a validation-error CLI test case.

Please add one test for schema-invalid (but syntactically valid) YAML to assert install_scenario_cmd exits cleanly with typer.Exit(1) and no traceback.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@otdf-sdk-mgr/tests/test_cli_scenario.py` around lines 107 - 126, Add a new
test in tests/test_cli_scenario.py that submits a syntactically valid but
schema-invalid YAML to install_scenario_cmd and asserts it exits cleanly with
typer.Exit(status=1) and no traceback; specifically, create a tmp_path scenario
file containing SCENARIO_YAML with a deliberate schema violation, call
install_scenario_cmd(scenario_path, ...) in a pytest.raises(typer.Exit) context
and assert the raised exception has exit_code == 1 and that no exception other
than typer.Exit is propagated (mirroring the pattern used in
test_install_scenario_writes_partial_manifest_on_failure), referencing
install_scenario_cmd and the tmp_path-based scenario file to locate where to add
the test.

otdf-sdk-mgr/tests/test_platform_installer.py (1)

8-23: ⚡ Quick win

Add a regression case for container-image-style refs.

Please add a parametrized case asserting container-image inputs are rejected (clear v1-not-supported error), so this behavior stays locked.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

otdf-sdk-mgr/src/otdf_sdk_mgr/cli_scenario.py (1)

57-72: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Handle non-YAML and schema-validation failures as CLI exits.

Only YAMLError is caught here. Read/type/validation failures can still escape with a traceback instead of a clean CLI error + exit code 1.

Proposed fix

+from pydantic import ValidationError
@@
-    from ruamel.yaml.error import YAMLError
+    from ruamel.yaml.error import YAMLError
@@
     try:
         raw = load_yaml_mapping(path)
-    except YAMLError as e:
+    except (OSError, YAMLError, ValueError) as e:
         typer.echo(f"Error: {path} is not valid YAML: {e}", err=True)
         raise typer.Exit(1)
@@
-    if kind == "Scenario":
-        scenario = Scenario.model_validate(raw)
-        instance = scenario.instance
-    elif kind == "Instance":
-        instance = Instance.model_validate(raw)
-    else:
-        typer.echo(f"Error: {path} has unknown kind {kind!r}", err=True)
-        raise typer.Exit(1)
+    try:
+        if kind == "Scenario":
+            scenario = Scenario.model_validate(raw)
+            instance = scenario.instance
+        elif kind == "Instance":
+            instance = Instance.model_validate(raw)
+        else:
+            typer.echo(f"Error: {path} has unknown kind {kind!r}", err=True)
+            raise typer.Exit(1)
+    except ValidationError as e:
+        typer.echo(f"Error: invalid manifest {path}: {e}", err=True)
+        raise typer.Exit(1)

As per coding guidelines "otdf-sdk-mgr/**/cli_*.py: Wrap library exceptions (InstallError, PlatformInstallError) at the CLI boundary and exit with typer.Exit(1) when adding new subcommands."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@otdf-sdk-mgr/src/otdf_sdk_mgr/cli_scenario.py` around lines 57 - 72, Wrap
both the YAML loading and model validation so non-YAML and schema/read/type
failures produce a clean CLI exit: surround the calls to
load_yaml_mapping(path), Scenario.model_validate(raw) and
Instance.model_validate(raw) with a try/except that catches validation/read
errors (e.g., ValidationError, FileNotFoundError and other Exceptions at the CLI
boundary) and on error call typer.echo with a clear message including the
exception and then raise typer.Exit(1); keep using the existing kind dispatch
and ensure the original YAMLError handling remains but add the broader/specific
exception handling around model_validate and loading to avoid tracebacks.

otdf-sdk-mgr/src/otdf_sdk_mgr/platform_installer.py (1)

98-104: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reject container-image refs before git resolution.

Inputs like ghcr.io/opentdf/platform:vX currently pass through because they contain /, so users get a later git failure instead of the intended v1-not-supported message.

Proposed fix

 def _resolve_platform_ref(version_or_ref: str) -> str:
@@
+    if (":" in version_or_ref or "@" in version_or_ref) and "/" in version_or_ref:
+        raise PlatformInstallError(
+            "container-image platform pins are not supported in v1; "
+            "use a git ref like v0.9.0 or service/v0.9.0"
+        )
     if "/" in version_or_ref or version_or_ref in ("main", "HEAD"):
         return version_or_ref

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@otdf-sdk-mgr/src/otdf_sdk_mgr/platform_installer.py` around lines 98 - 104,
The current check returns version_or_ref whenever it contains "/", which wrongly
treats container image refs like "ghcr.io/org/image:tag" as git refs; update the
condition so it only returns early for git-like paths and not container
images—i.e., replace the simple `if "/" in version_or_ref or version_or_ref in
("main", "HEAD"):` with a check that first rejects container-image patterns
(implement a helper like looks_like_container_image(version_or_ref) that detects
a registry domain in the first segment (contains a dot, e.g. "ghcr.io"), or an
image tag/digest (contains ":" after the last "/" or "@" digest)), and only
return version_or_ref for "/" cases when looks_like_container_image(...) is
false; keep the existing hex-sha length check and the final `return
f"{infix}/{normalize_version(version_or_ref)}"` as-is so container-image refs
fall through to the git resolution path and trigger the correct v1-not-supported
behavior.

otdf-sdk-mgr/tests/test_cli_scenario.py (1)

107-126: ⚡ Quick win

Add a regression test for invalid manifest handling.

Please add a case where schema validation fails (or YAML/top-level mapping is invalid) and assert the command exits via typer.Exit(1) without an unhandled traceback.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@otdf-sdk-mgr/tests/test_cli_scenario.py` around lines 107 - 126, Add a new
regression test in tests/test_cli_scenario.py that supplies an invalid scenario
manifest (e.g., malformed YAML or wrong top-level type) and invokes
install_scenario_cmd, asserting it raises typer.Exit with exit code 1 and does
not propagate a traceback; reuse the test pattern in
test_install_scenario_writes_partial_manifest_on_failure to create a tmp_path
scenario file, patch dependencies used by install_scenario_cmd (like
otdf_sdk_mgr.cli_scenario.install_platform_release, install_helper_scripts,
install_release) as needed so the failure is triggered solely by manifest/schema
validation, and assert the command exits via typer.Exit(1) rather than raising
other exceptions.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes